Repository navigation
resolver: pass absolute path to browser-map check for main field and index - #36620
Conversation
…index
check_browser_map<AbsolutePath> expects an absolute path that starts with
the package directory so it can strip that prefix and look up the
package-relative key in the browser map. load_from_main_field and
load_as_index_with_browser_remapping were passing the relative path
("./lib/index", "index") instead, so the starts_with guard rejected it
and the browser map was never consulted for main-field values.
This meant a package.json like
"main": "./lib/index",
"browser": {"./lib/index": "./dist/pkg.min.js"}
resolved to lib/index.js for browser targets instead of the prebuilt
bundle. jszip uses exactly this pattern and was pulling in the full
lib/ tree plus node polyfills, producing a 470KB bundle where esbuild
produces 165KB.
Compute the absolute path before the browser-map check, matching
esbuild's loadMainField / loadAsIndexWithBrowserRemapping.
Fixes #22797
Fixes #21128
|
Warning Review limit reached
Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…t the browser scope
The browser-map check in load_as_index_with_browser_remapping was
previously dead code (the relative "index" never started with the
absolute package dir), so two latent issues in its remap branch never
fired:
- the disabled path was built with Path::init over a threadlocal scratch
buffer, leaving the returned MatchResult pointing at memory the next
resolve overwrites;
- the remap target was joined against the subdirectory being indexed
instead of the package that owns the browser field, so a subpath
directory import like require('demo-pkg/sub') with a remap in
demo-pkg/package.json resolved to demo-pkg/sub/<remap> and failed.
Intern the disabled path via abs_alloc (matching the sibling in
load_from_main_field) and join the remap against browser_scope.abs_path
in both functions.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Both earlier findings (scratch-buffer intern for the disabled-index path, and joining the remap against browser_scope.abs_path) are correctly addressed in d8429ca, and the added BrowserMapSubpathDirIndex/BrowserMapSubpathDirIndexDisabled tests pin the newly-live branches. Deferring to a human only because this activates previously-dead resolver branches in a hot path — the change itself looks right.
What was reviewed:
- Verified
check_browser_mapwrites onlybufs!(check_browser_map)/bufs!(abs_to_rel), so the newfield_abs_path/index_abs_pathborrows intobufs!(field_abs_path)/bufs!(remap_path)are not clobbered before use. - Confirmed the disabled branch at 5276 now uses
abs_alloc(interned), matching the sibling at 5042. - Checked the six new
itBundledcases cover the extension-permutation matrix, thefalse(disabled) case for both call sites, and the subpath-dir remap that would have regressed with the wrong join base.
Extended reasoning...
Overview
The PR fixes browser-map handling for the "main" field and implicit index when the browser-map key is written without an extension (jszip-shaped packages). Two functions in src/resolver/resolver.rs — load_from_main_field and load_as_index_with_browser_remapping — now compute an absolute path before calling check_browser_map::<AbsolutePath>, and resolve the remap value against browser_scope.abs_path instead of the local directory. Six itBundled tests are added to test/bundler/esbuild/packagejson.test.ts.
Prior findings addressed
My earlier pass flagged (1) the remap.is_empty() branch storing a slice into the threadlocal bufs!(remap_path) scratch directly in MatchResult.path_pair, and (2) the non-empty remap being joined against the subdirectory path rather than the browser-scope directory, which would have turned import 'demo-pkg/sub' into a hard resolve error. d8429ca fixes both: the disabled path is interned via abs_alloc(&index_paths), and both call sites now join against browser_scope.abs_path (matching esbuild's r.fs.Join(browserScope.absPath, *remapped)). New tests BrowserMapSubpathDirIndex and BrowserMapSubpathDirIndexDisabled cover exactly the case the second finding described.
Security risks
None. This is bundler/resolver path-selection logic; browser-map values are package-supplied and were already consumed elsewhere (the post-resolution check at 2135/2357). No new untrusted input surface.
Level of scrutiny
Moderate-to-high. resolver.rs is production-critical and threadlocal-scratch-heavy; the change activates two branches that were provably dead before (b"index" / field_rel_path never satisfied the starts_with(abs_path) guard). Both dead branches carried latent bugs that this PR then fixed after review. The diff is small (~30 lines net in resolver.rs) and now aligns line-for-line with the esbuild reference, but the fact that activating this code required two follow-up fixes argues for a maintainer familiar with the resolver's buffer discipline to sign off.
Other factors
I re-checked buffer aliasing after the fix: check_browser_map writes only bufs!(check_browser_map) and bufs!(abs_to_rel), so passing a slice of bufs!(field_abs_path) / bufs!(remap_path) into it is safe; the subsequent overwrite of bufs!(field_abs_path) at 5057 uses [browser_scope.abs_path, remap] as inputs, neither of which borrows that buffer. Test coverage is thorough (permutation matrix matches the esbuild table in the description; both disabled-path variants; the ancestor-scope subpath case). No outstanding reviewer comments.
There was a problem hiding this comment.
Both earlier findings are addressed — the disabled-index path is now interned via abs_alloc, and both remap joins resolve against browser_scope.abs_path. I didn't find anything new this pass. That said, this activates two previously-dead resolver branches for browser targets; worth a human sign-off given the ecosystem-wide reach of resolution changes.
Checked: check_browser_map writes only bufs!(check_browser_map)/bufs!(abs_to_rel), so the new index_abs_path in bufs!(remap_path) isn't clobbered before use; the field_abs_path re-join reads only browser_scope.abs_path and remap (neither in field_abs_path's buffer); abs_alloc routes through DirnameStore so the disabled record outlives the frame. New tests cover the jszip shape, the 4-way ext matrix, disabled, dir-main index, and the subpath-dir remap that would have regressed with the wrong join base.
Extended reasoning...
Overview
Fixes #22797 / #21128: load_from_main_field and load_as_index_with_browser_remapping were passing a relative path to check_browser_map::<AbsolutePath>, whose first line is if !input_path.starts_with(dir_info.abs_path) { return None } — so the entire remap branch in both functions was dead code and packages like jszip@3.10.1 ("main": "./lib/index", "browser": {"./lib/index": "./dist/jszip.min.js"}) bundled the Node graph instead of the prebuilt browser file. The PR computes the absolute path first (matching esbuild's fieldAbsPath := r.fs.Join(path, fieldRelPath)) and passes that. Making the branch live exposed two latent bugs in the previously-dead code, which I flagged in the first pass and which were fixed in d8429ca: (1) the disabled arm stored a scratch-buffer slice into MatchResult — now interned via abs_alloc, matching the sibling at 5042; (2) the non-empty remap was joined against the subdirectory being indexed instead of browser_scope.abs_path — now fixed in both functions, with a new BrowserMapSubpathDirIndex test covering the case that would have hard-errored.
Security risks
None. This is module-resolution logic for the "browser" field in package.json; no auth/crypto/permissions surface. Inputs come from on-disk package.json files already trusted for resolution.
Level of scrutiny
High. The resolver is production-critical and subtle; a wrong remap here changes which file gets bundled for every browser build that hits the pattern. This PR specifically activates two code paths that have never executed before, which is exactly where latent bugs hide (and did — two were found and fixed during review). The change is small and tracks the esbuild reference closely, but resolver behavior changes deserve a maintainer's eyes.
Other factors
- Six new
itBundledtests inpackagejson.test.tscover the reported bug, the full main×browser extension permutation matrix (verified against esbuild in the PR description), thefalsedisabled case, the directory-main/indexcase, and the subpath-directory remap (both remap-to-file and disabled). The subpath test directly exercises the join-base fix. - I re-checked buffer aliasing:
check_browser_mapwritesbufs!(check_browser_map)/bufs!(abs_to_rel), distinct from the newbufs!(remap_path)/bufs!(field_abs_path)writes; the re-join at 5057 reads only internedbrowser_scope.abs_pathand the browser-map-ownedremap, not the buffer being overwritten;abs_allocusesjoin_abs_string's separate threadlocal, soindex_paths(which borrowsremap_path_trailing_slash) is not clobbered. - The
.expect("unreachable")onabs_allocmatches the existing sibling at line 5042. - The one CI failure (
test/regression/issue/36577.test.tson Windows x64) is unrelated to this change. - All three prior inline threads (my two bug reports + the comment-cop nit) are resolved.
What
When a package's
"main"field and the matching"browser"map key are both written without an extension,bun build --target=browserignores the browser remap and bundles the Node entrypoint.mkdir -p node_modules/pkg/{lib,dist} cat > node_modules/pkg/package.json <<'JSON' {"name":"pkg","main":"./lib/index","browser":{"./lib/index":"./dist/pkg.min.js"}} JSON echo 'module.exports="LIB"' > node_modules/pkg/lib/index.js echo 'module.exports="PREBUILT"' > node_modules/pkg/dist/pkg.min.js echo 'console.log(require("pkg"))' > entry.js bun build entry.js --target=browser # bundles "LIB", should bundle "PREBUILT"Real-world impact:
import "jszip"withjszip@3.10.1, which ships exactly this pattern to point browsers at a prebuiltdist/jszip.min.js. Bun walks the fulllib/graph instead and ends up bundling node stream/buffer/events polyfills:bun buildbeforebun buildafterWhy
check_browser_map::<AbsolutePath>expects an absolute path so it can strip the package-directory prefix and look up the package-relative key.load_from_main_fieldandload_as_index_with_browser_remappingwere passing the relative main-field value ("./lib/index","index") instead, so thestarts_with(abs_path)guard returnedNoneand the whole remap branch in both functions was dead code. The only match that ever worked was the post-resolution check, which sees the resolved path with its extension and therefore only matches when the browser key also carries the extension.esbuild computes
fieldAbsPath := r.fs.Join(path, fieldRelPath)before callingcheckBrowserMaphere; this change brings both functions in line with that.Making the remap branch reachable exposed two latent issues in the previously-dead code, also fixed here:
"browser": {"./x": false}) arm stored a slice into a threadlocal scratch buffer directly in the returnedMatchResult; it now interns viaabs_alloclike the sibling inload_from_main_field;require('demo-pkg/sub')with"./sub/index.js": "./browser-sub.js"indemo-pkg/package.json) would have looked indemo-pkg/sub/browser-sub.jsand failed. Both functions now join againstbrowser_scope.abs_path.Verification
The four
main/browserextension permutations now match esbuild exactly:./lib/index.js./lib/index.js./lib/index./lib/index.js./lib/index.js./lib/index./lib/index./lib/indexNew
itBundledtests intest/bundler/esbuild/packagejson.test.tscover the jszip-shaped remap, the permutation matrix above, thefalse(disabled) case, the directory-main/indexremap, and the subpath-directory remap. All fail on the released build and pass with this change; the rest of the file is unchanged (80 pass, 9 todo).Not in this PR
The
packagejson/BrowserNodeModulesNoExttodo test exercises bare subpath file imports (import 'demo-pkg/no-ext'). That needs a browser-map check insideload_node_modulesbeforeload_as_file_or_directory, which is missing entirely rather than mis-parameterised, and is left for a follow-up.Fixes #22797
Fixes #21128